Skip to content

Conversation

@zbraniecki
Copy link
Member

Depends on #7112 .
Partially resolves #5869 implementing refactor plus documentation, and fixing #6574 .

@zbraniecki zbraniecki requested review from a team and nciric as code owners October 16, 2025 15:06
@zbraniecki zbraniecki force-pushed the host_info branch 10 times, most recently from 7b7864e to 84e83fe Compare October 17, 2025 00:24
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API overall looks quite straightforward

@zbraniecki
Copy link
Member Author

I'll need help figuring out how to avoid enabling gnome in our CI/CD at least. Right now I tried diabling it in Cargo.toml all-features section but it still gets enabled in many CI/CD commands. :(

//! | Date format | ⚠️ | ⚠️ | ⚠️ | ⚠️ | ⚠️ |
//! | Number format | ⚠️ | ⚠️ | ⚠️ | ⚠️ | ⚠️ |
//!
//! <sup>(1)</sup> In case of Linux different DE's such as Gnoem and KDE are supported together.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's a "DE"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desktop Environment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please spell out. also Gnome has a typo

@dpulls
Copy link

dpulls bot commented Oct 22, 2025

🎉 All dependencies have been resolved !

@zbraniecki
Copy link
Member Author

Thanks all. Applied feedback and rebased on top of main. I'm still not sure how to handle working around --all-features trying to pull gio on Linux.

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I also don't know how to fix the --all-features test. Does the gio provide any guidance for use in libraries?

@Manishearth
Copy link
Member

  • @robertbastian We should make the gio crate build when glib isn't present.
  • @Manishearth This isn't target selection. It is whether or not glib is installed. Very unlikely to be accepted upstream

Four solutions:

  • We add an all_features_internal feature to the crate
  • We install glib on CI
  • We use --exclude with --all-features and CAF's exclude directives for cargo-all-features. We add a separate CI makefile job for this crate.
  • Remove it from the workspace?

Discussion:

  • @zbraniecki The metadata thing doesn't work
  • @Manishearth --exclude does. These are two separate things: CAF vs cargo --all-features.
  • @zbaniecki Ah!
  • @sffc Does the crate itself have features, or is it the dependency problem? Can we use a build.rs to fix the problem, or upstream a build.rs file into the crate (not RUSTFLAGS)?
  • @Manishearth No I think that is unlikely to accepted, too. The package exists because it interfaces with glib. It seems bad for it to silently build when it definitely won't work at runtime.
  • @robertbastian Install glib-sys on CI isn't a solution since it impacts contributors
  • @robertbastian What if we CI'd on mac
  • @Manishearth still same problem with local running for contributors
  • @sffc There is a correct solution: to do it out of workspace, what we do for harfbuzz
  • @robertbastian That's an example of usage, not a library.
  • @robertbastian If you ever notice issues in this crate when you make PRs you have to wait for CI
  • @Manishearth If you're working on this crate you should CAF anyway.
  • @sffc icu_host_info should depend on public API of icu_locale_core so the CI should only break in cases where semver breaks
  • @robertbastian Won't workspace mean it's removed from the metacrate?
  • @Manishearth can't be in the metacrate as a util crate anyway, and we definitely don't want sys deps in the metacrate
  • @sffc we don't have consensus on metacrate. Longer term, we should investigate whether we can adopt a solution more like harfbuzz-traits
  • @robertbastian it should be in the msrv-check job, the test-cargo job is for Rust examples, not for published crates

Solution:

  • Remove it from the workspace, add a separate CI Makefile task
  • Stuff it in the harfbuzz CI task (test-cargo? maybe rename that task)

@zbraniecki zbraniecki force-pushed the host_info branch 3 times, most recently from c68477c to 8c0c88c Compare October 28, 2025 05:24
@zbraniecki
Copy link
Member Author

@Manishearth the PR is ready. Can you guide me through where to inject it into CI? We can also do this in a separate PR

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use merge commits instead of rebase if there are active reviews. GitHub does not support seeing a diff since the last review if the branch was force-pushed.

@Manishearth Manishearth requested a review from sffc as a code owner October 30, 2025 06:06
@Manishearth
Copy link
Member

I took a stab at writing CI. I added it to the check task like Robert suggested since the cargo task checks tutorials.

@zbraniecki it would be nice if we could actually cargo test here. Is there perhaps a dependency we should install on the builders? I'm not 100% sure of what the problem is: which commands don't work and on which targets.

@sffc
Copy link
Member

sffc commented Oct 30, 2025

I thought the CI was going to be augmenting the job that tests harfbuzz to also install glib and test host info

@sffc
Copy link
Member

sffc commented Oct 30, 2025

How about clippy?

It seems like this is also something where we actually care a lot about building on the matrix of platforms?

@Manishearth
Copy link
Member

I thought the CI was going to be augmenting the job that tests harfbuzz to also install glib and test host info

We could; that task currently tests tutorials so it's a bit awkward to stick this in there. But maybe test-cargo should call test-tutorials and test-hostinfo? That's an easy change.

We never really agreed on what CI we wanted; Zibi just asked for help on how to make CI actually work.

How about clippy?

It seems like this is also something where we actually care a lot about building on the matrix of platforms?

Probably! I just did the first pass since Zibi wanted help with the makefiles. I don't have a full understanding of what is and isn't broken here, and I don't have the time to play with CI to figure it out. There's now a makefile task dedicated to this test that Zibi or someone else can expand and experiment with.

@Manishearth
Copy link
Member

I think what we should probably do is rename ci-job-cargo to ci-job-nonworkspace, have it call a test-tutorials and test-hostinfo

test-hostinfo runs check and test.

we also have ci-job-clippy call clippy-hostinfo.

I think that makes sense?

@zbraniecki
Copy link
Member Author

@Manishearth the only dep that is required to run tests is on Linux platform we need to install gio library to be able to run --all-features.
I agree that it may be nice to test it on all platforms.

@Manishearth
Copy link
Member

If we want to test it on all platforms we probably want a subtask of the test task

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely merge this and iterate on this after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Publish env_preferences?

4 participants